From 9514dafd0e888779b56dab22cdf34773bcc2dfa6 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Wed, 25 Jun 2014 11:48:38 +1000 Subject: [PATCH] Handle misformatted versions with a nicer error message. --- src/cargo/core/package_id.rs | 83 ++++++++++++++++++++++++++---------- src/cargo/core/resolver.rs | 8 ++-- src/cargo/util/toml.rs | 5 ++- tests/test_cargo_compile.rs | 18 ++++++++ 4 files changed, 85 insertions(+), 29 deletions(-) diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index 40a30aadf..d96e21547 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -10,43 +10,46 @@ use serialize::{ Decoder }; -use util::{CargoError, FromError}; +use util::{CargoResult, CargoError}; trait ToVersion { - fn to_version(self) -> Option; + fn to_version(self) -> Result; } impl ToVersion for semver::Version { - fn to_version(self) -> Option { - Some(self) + fn to_version(self) -> Result { + Ok(self) } } impl<'a> ToVersion for &'a str { - fn to_version(self) -> Option { - semver::parse(self) + fn to_version(self) -> Result { + match semver::parse(self) { + Some(v) => Ok(v), + None => Err(format!("cannot parse '{}' as a semver", self)), + } } } trait ToUrl { - fn to_url(self) -> Option; + fn to_url(self) -> Result; } impl<'a> ToUrl for &'a str { - fn to_url(self) -> Option { - url::from_str(self).ok() + fn to_url(self) -> Result { + url::from_str(self) } } impl ToUrl for Url { - fn to_url(self) -> Option { - Some(self) + fn to_url(self) -> Result { + Ok(self) } } impl<'a> ToUrl for &'a Url { - fn to_url(self) -> Option { - Some(self.clone()) + fn to_url(self) -> Result { + Ok(self.clone()) } } @@ -57,14 +60,32 @@ pub struct PackageId { namespace: Url } +#[deriving(Clone, Show, PartialEq)] +pub enum PackageIdError { + InvalidVersion(String), + InvalidNamespace(String) +} + +impl CargoError for PackageIdError { + fn description(&self) -> String { + match *self { + InvalidVersion(ref v) => format!("invalid version: {}", *v), + InvalidNamespace(ref ns) => format!("invalid namespace: {}", *ns), + } + } + fn is_human(&self) -> bool { true } +} + impl PackageId { pub fn new(name: &str, version: T, - namespace: U) -> PackageId { - PackageId { + namespace: U) -> CargoResult { + let v = try!(version.to_version().map_err(InvalidVersion)); + let ns = try!(namespace.to_url().map_err(InvalidNamespace)); + Ok(PackageId { name: name.to_str(), - version: version.to_version().unwrap(), - namespace: namespace.to_url().unwrap() - } + version: v, + namespace: ns + }) } pub fn get_name<'a>(&'a self) -> &'a str { @@ -94,14 +115,17 @@ impl Show for PackageId { } } -impl, D: Decoder> Decodable for PackageId { - fn decode(d: &mut D) -> Result { - let vector: Vec = try!(Decodable::decode(d)); +impl>> Decodable> for PackageId { + fn decode(d: &mut D) -> Result> { + let vector: Vec = match Decodable::decode(d) { + Ok(v) => v, + Err(e) => return Err(e.to_error()) + }; - Ok(PackageId::new( + PackageId::new( vector.get(0).as_slice(), vector.get(1).as_slice(), - vector.get(2).as_slice())) + vector.get(2).as_slice()) } } @@ -111,3 +135,16 @@ impl> Encodable for PackageId { self.namespace.to_str()).encode(e) } } + +#[cfg(test)] +mod tests { + use super::{PackageId, central_repo}; + + #[test] + fn invalid_version_handled_nicely() { + assert!(PackageId::new("foo", "1.0", central_repo).is_err()); + assert!(PackageId::new("foo", "1", central_repo).is_err()); + assert!(PackageId::new("foo", "bar", central_repo).is_err()); + assert!(PackageId::new("foo", "", central_repo).is_err()); + } +} diff --git a/src/cargo/core/resolver.rs b/src/cargo/core/resolver.rs index e7d04427f..da0dcb834 100644 --- a/src/cargo/core/resolver.rs +++ b/src/cargo/core/resolver.rs @@ -88,19 +88,19 @@ mod test { Dependency::parse(*s, Some("1.0.0"), &source_id).unwrap() }).collect(); Summary::new(&PackageId::new($name, "1.0.0", - "http://www.example.com/"), + "http://www.example.com/").unwrap(), d.as_slice()) } ); ($name:expr) => ( Summary::new(&PackageId::new($name, "1.0.0", - "http://www.example.com/"), []) + "http://www.example.com/").unwrap(), []) ) ) fn pkg(name: &str) -> Summary { - Summary::new(&PackageId::new(name, "1.0.0", "http://www.example.com/"), + Summary::new(&PackageId::new(name, "1.0.0", "http://www.example.com/").unwrap(), &[]) } @@ -116,7 +116,7 @@ mod test { fn names(names: &[&'static str]) -> Vec { names.iter() - .map(|name| PackageId::new(*name, "1.0.0", "http://www.example.com/")) + .map(|name| PackageId::new(*name, "1.0.0", "http://www.example.com/").unwrap()) .collect() } diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs index 86ce05f72..8776eac28 100644 --- a/src/cargo/util/toml.rs +++ b/src/cargo/util/toml.rs @@ -88,13 +88,14 @@ pub struct TomlManifest { #[deriving(Decodable,Encodable,PartialEq,Clone,Show)] pub struct TomlProject { pub name: String, + // FIXME #54: should be a Version to be able to be Decodable'd directly. pub version: String, pub authors: Vec, build: Option, } impl TomlProject { - pub fn to_package_id(&self, namespace: &Url) -> PackageId { + pub fn to_package_id(&self, namespace: &Url) -> CargoResult { PackageId::new(self.name.as_slice(), self.version.as_slice(), namespace) } } @@ -161,7 +162,7 @@ impl TomlManifest { let project = try!(project.require(|| human("No `package` or `project` section found."))); Ok((Manifest::new( - &Summary::new(&project.to_package_id(source_id.get_url()), + &Summary::new(&try!(project.to_package_id(source_id.get_url())), deps.as_slice()), targets.as_slice(), &Path::new("target"), diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs index 57bd17dac..6354ad422 100644 --- a/tests/test_cargo_compile.rs +++ b/tests/test_cargo_compile.rs @@ -51,6 +51,7 @@ test!(cargo_compile_with_invalid_manifest { No `package` or `project` section found.\n")) }) + test!(cargo_compile_with_invalid_manifest2 { let p = project("foo") .file("Cargo.toml", r" @@ -65,6 +66,23 @@ test!(cargo_compile_with_invalid_manifest2 { Cargo.toml:3:19-3:20 expected a value\n\n")) }) +test!(cargo_compile_with_invalid_version { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + authors = [] + version = "1.0" + "#); + + assert_that(p.cargo_process("cargo-build"), + execs() + .with_status(101) + .with_stderr("Cargo.toml is not a valid manifest\n\n\ + invalid version: cannot parse '1.0' as a semver\n")) + +}) + test!(cargo_compile_without_manifest { let p = project("foo"); -- 2.30.2